fix(native-eval): reject invalid harness runs - #64
Draft
vincentkoc wants to merge 1 commit into
Draft
Conversation
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: found issues before merge. Reviewed August 3, 2026, 4:13 PM ET / 20:13 UTC. ClawSweeper reviewWhat this changesThe PR records native-evaluation execution outcomes, excludes fully invalid runs from score-eligible aggregates, and requires fleet recovery to use an archived run exit status instead of inferring success from result coverage and a final checkpoint. Merge readiness⛔ Blocked by patch quality or review findings - 6 items remain Keep this PR open: its central invalid-run rejection is valuable, but the current patch removes an existing recovery path for complete legacy exports that lack an archived exit status, turning them into failed/rerun jobs. The author is a repository member, so the compatibility policy needs explicit maintainer judgment rather than automatic closure. Priority: P1 Review scores
Verification
How this fits togetherNative evaluation runs task trials through a harness, saves trial results and final artifacts, then aggregates scores and lets fleet recovery finalize exported runs. This PR changes how execution validity reaches both score eligibility and the fleet’s recovered completion state. flowchart LR
A[Native evaluation plan] --> B[Harness trial execution]
B --> C[Trial results and execution outcomes]
C --> D[Final archive and exit-status artifact]
C --> E[Score aggregation]
D --> F[Fleet recovery]
E --> G[Score-eligible report]
F --> H[Completed or rerun state]
Decision needed
Why: The patch intentionally removes current recovery behavior, but repository evidence does not establish whether older complete exports may safely retain that path or must be invalidated; this is an upgrade-policy choice, not a mechanical review preference. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep explicit archived exit status as the required contract for new runs, while preserving a narrowly identified legacy recovery path or adding a documented migration/invalid-archive policy with tests for both archive generations. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main has a focused fleet test for a verified full-coverage archive with a final checkpoint and no archived exit status, while this PR changes that same case from completed to failed. The review is read-only, so it did not execute the test suite. Is this the best way to solve the issue? No: rejecting invalid new harness runs is the right direction, but deleting the only recovery path for legacy complete exports is not the narrowest safe solution without an explicit archive compatibility decision. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found, but no applicable review policy affected this item. Codex review notes: model internal, reasoning high; reviewed against 884dd1bb5511. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (22 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
status: ⏳ waiting on author
clawsweeper
Bot
added
rating: 🦪 silver shellfish
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Records structured native-eval execution outcomes and rejects runs where every
trial failed at the harness, infrastructure, or verifier layer.
Why?
Verifier rewards are useful diagnostics, but they are not proof that the
benchmark harness executed correctly. Fully invalid runs were being presented
as completed score data, and fleet recovery could infer success without an
archived exit status.
Fixes #63
Changes
Tests
python -m pytest -qpasses on Blacksmith Testboxpython -m ruff check clawbench app.py scripts testspasses on Blacksmith TestboxFocused proof: 117 tests passed; Ruff clean. Autoreview reported no actionable
findings.